Skip to content

Conversation

@bugraoz93
Copy link
Contributor

closes: #54090
Removing the feature, and we will keep it under the administrative airflow CLI and will move it afterwards.


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in airflow-core/newsfragments.

@bugraoz93 bugraoz93 requested review from kaxil and potiuk as code owners August 9, 2025 00:26
@bugraoz93 bugraoz93 changed the title feat(airflowctl): remove import/export connections (airflowctl): remove import/export connections Aug 9, 2025
@bugraoz93 bugraoz93 force-pushed the airflowctl/54090/remove-export-import-connection branch from da58c59 to 5182c1e Compare August 9, 2025 00:27
@ashb
Copy link
Member

ashb commented Aug 9, 2025

There's no need for us remove import. Rename it to bulk-create if you want, but why remove it?

@potiuk
Copy link
Member

potiuk commented Aug 9, 2025

There's no need for us remove import. Rename it to bulk-create if you want, but why remove it?

I think if we leave it, we have to explain how to create a good "export" file to be able to "import" - we could explain in detail that you need to run "export" using "cli" command rather than airflowctl. It would be quite confusing to see "import" option and no explanation on how to create the file.

I think we could leave import if we explain that - at least for now we remove export, and explain the relationship to "airflow" CLI, but I find it fairly confusing.

Or do you think @ashb that people will be preparing such import files manually or in other ways? I find it quite improbable, and if we expect it to happen then we likely need to describe the exact format of the import file so that they can do it (but again - I do not find it too likely for users to do it - they would rather use "airflow connection export" after logging in to the airflow pod/contianer/venv - and we should at the very least tell it ito them?

Or am I missing something?

@jscheffl
Copy link
Contributor

jscheffl commented Aug 9, 2025

There's no need for us remove import. Rename it to bulk-create if you want, but why remove it?

I think if we leave it, we have to explain how to create a good "export" file to be able to "import" - we could explain in detail that you need to run "export" using "cli" command rather than airflowctl. It would be quite confusing to see "import" option and no explanation on how to create the file.

I think we could leave import if we explain that - at least for now we remove export, and explain the relationship to "airflow" CLI, but I find it fairly confusing.

Or do you think @ashb that people will be preparing such import files manually or in other ways? I find it quite improbable, and if we expect it to happen then we likely need to describe the exact format of the import file so that they can do it (but again - I do not find it too likely for users to do it - they would rather use "airflow connection export" after logging in to the airflow pod/contianer/venv - and we should at the very least tell it ito them?

Or am I missing something?

I see and agree to both of your statements. But if we want admins in future to use airflowctl as well might be "handy" similar like kubectl apply -f ... ... and importing is perfectly secure. Yeah might be confusing first-place but is good if you can also "from remote" import? Better than forcing getting your definitions on the server side to run import on CLI only.

@potiuk
Copy link
Member

potiuk commented Aug 10, 2025

I see and agree to both of your statements. But if we want admins in future to use airflowctl as well might be "handy" similar like kubectl apply -f ... ... and importing is perfectly secure. Yeah might be confusing first-place but is good if you can also "from remote" import? Better than forcing getting your definitions on the server side to run import on CLI only.

Yeah. I am fine to leave it as long as we say "and here is how you can do export". Also I think we should be VERY SPECIFIC when you export connections that the export file should be highly protected. If people will get used to the fact that connection details are hidden, they might not be aware that this file containst such highly sensitive data. I know it's maybe a bit of "hand-holding" the deployment managers, but the "secure-by-design" principle is now an important part of even the CRA framework. Everything that has potential dangerous security consequences should be VERY clearly marked. as such. I would even say that our 'export" feature should by default ask the user to confirm it and explain dangers involved with having to secure the file.

That's why I also it might not be that outrageous to even ask the user - when exporting the file - to provide a passphrase that the export should be encrypted with, and do not allow to run export without long-enough passphrase (and allow for example to specify the passphrase via ENV VARS to allow automation)

I think that would be ultimate way of teliing the Deployment Manager that they shoudl be careful.

The encryption does not have to be done now as part of it of course, but I would say, it should be the end-goal.

@bugraoz93
Copy link
Contributor Author

The main goal should have both features at once, maybe not now, but we should somehow allow users/administrators to interact with airflowctl for these.
The decision to remove them was based on the fact that we don't have much time for release, and we can add it back in Beta 2 or before RC1. This was the easier path to take. If we explain it thoroughly, it should be sufficient.

I have added the import back. It is compatible with the airflow CLI. I have tested exporting from there and importing using airflowctl. I have added this detail to the tooltip

Screenshot from 2025-08-11 10-58-56

@bugraoz93 bugraoz93 force-pushed the airflowctl/54090/remove-export-import-connection branch from 5182c1e to 6b9db77 Compare August 11, 2025 09:08
@potiuk
Copy link
Member

potiuk commented Aug 11, 2025

That will do, yes :)

@bugraoz93 bugraoz93 force-pushed the airflowctl/54090/remove-export-import-connection branch from 6b9db77 to 0a8027a Compare August 12, 2025 09:08
@bugraoz93 bugraoz93 merged commit 9885172 into apache:main Aug 12, 2025
48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Export import for connections for airflowctl is broken in main

4 participants